Skip to content

Conversation

@alex35mil
Copy link
Collaborator

Major bump.

@alex35mil alex35mil requested a review from cknitt May 5, 2020 12:39
values={
"bold": text => <strong> text </strong>,
"italic": text => <em> text </em>,
"combined": (italicBold, text) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can model (...chunks) => <div> {chunks} </div> in BuckleScript.

@alex35mil
Copy link
Collaborator Author

Published bs-react-intl@2.0.0-beta.1 under next tag.

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! LGTM, although I didn't check every prop in detail. Just two small remarks.


[@bs.get] external type_: t => string = "type";
[@bs.get] external value: t => string = "value";
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With objects as records in BS 7, I would model this as a record.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done.

We prolly should update message & translation types as well. But the former would require changes in the extractor AFAICT.

Copy link
Member

@cknitt cknitt May 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's a good idea! I can look into updating the extractor sometime next week.

This should allow for a better extractor API, too. Instead of

let msg = [@intl.messages] {
                             "helloWorld": {
                               "id": "MyComponent.helloWorld",
                               "defaultMessage": "Hello, world!",
                             },
                           };

(strange indentation by refmt...) we could e.g. have

[@intl.messages]
module Msg = {
  open ReactIntl;

  let helloWorld = {id: "MyComponent.helloWorld", defaultMessage: "Hello, world!"};
};

😃

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds perfect.

AFAICT we can even drop sub-module and define annotation on the module level:

[@intl.messages];

open ReactIntl;

let helloWorld = {id: "MyComponent.helloWorld", defaultMessage: "Hello, world!"};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you have a "global message module" (containing all the application's messages), that makes sense. I can look into adding this usage pattern to the extractor, too.

In my apps, I prefer having smaller submodules that contain just the messages used by their parent module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexfedoseev bs-react-intl-extractor master now supports this pattern. 🎉

Can you try it and, if everything works for you, change the message and translation types to records?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Will do tomorrow and ping you back 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexfedoseev Did you have a chance to look into this? Alternatively, we can merge this PR and I can open a new one changing message and translation to records.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cknitt Sorry, I got hit by the unexpected deadline on the client project and I'll be busy until the end of this month. If you have time on this, it would be great. Otherwise, I'll be dealing with my OSS debts right after I finish with the MVP and will take care of this. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Will do this later today / this weekend. Will merge this PR and open a separate one.

package.json Outdated
"react": "16.13.1",
"react-dom": "16.13.1",
"react-intl": "4.5.2",
"reason-react": "0.8.0-dev.4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.8.0 release is out now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants